GODRIVER-4034 Update the built-in AWS credential interfaces to match aws-sdk-go-v2.#2481
GODRIVER-4034 Update the built-in AWS credential interfaces to match aws-sdk-go-v2.#2481qingyang-hu wants to merge 1 commit into
Conversation
|
There is an existing patch(es) for this commit SHA: Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'. |
qingyang-hu
force-pushed
the
godriver3567
branch
from
9a0ac7f to
d35eac3
Compare
🧪 Performance ResultsCommit SHA: d35eac3The following benchmark tests for version 6a563c3d883db50007a6bc60 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change Report./v2/x/mongo/drivercompatible changesAWSCredentialsProvider: added |
qingyang-hu
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the driver’s AWS/Azure credential plumbing to support context-aware credential retrieval, adds an AWS credentials provider hook on driver.Cred, and refactors the internal AWS auth flow to use the v4 signer directly.
Changes:
- Add
AWSCredentialsProvidersupport tox/mongo/driver.Credand wire it into MongoDB-AWS authentication. - Refactor internal AWS/Azure credential providers to use
Retrieve(ctx)/Get(ctx)and move expiration tracking intocredentials.Value. - Update internal credentials caching/chain logic and adjust related tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| x/mongo/driver/driver.go | Adds AWSCredentialsProvider to Cred and defines the provider interface. |
| x/mongo/driver/auth/mongodbaws.go | Wires optional custom AWS provider into the provider chain and uses v4 signer path. |
| x/mongo/driver/auth/creds/credscaching_test.go | Updates AWS credential caching test expectations for invalid/expired credentials. |
| x/mongo/driver/auth/creds/azurecreds.go | Switches Azure credential retrieval to Credentials.Get(ctx). |
| x/mongo/driver/auth/creds/awscreds.go | Switches AWS credential retrieval to Credentials.Get(ctx). |
| x/mongo/driver/auth/aws_sasl_adapter.go | Refactors SASL client to sign requests via v4 signer instead of the old conversation wrapper. |
| internal/credproviders/static_provider.go | Updates static provider to implement Retrieve(context.Context). |
| internal/credproviders/imds_provider.go | Refactors Azure IMDS provider to return Value expiration via fields instead of internal state. |
| internal/credproviders/env_provider.go | Refactors env provider to context signature and uses CanExpire/Expires semantics for invalid creds. |
| internal/credproviders/ecs_provider.go | Refactors ECS provider to context signature and uses CanExpire/Expires in returned Value. |
| internal/credproviders/ec2_provider.go | Refactors EC2 provider to context signature and uses CanExpire/Expires in returned Value. |
| internal/credproviders/assume_role_provider.go | Refactors assume-role provider to context signature and uses CanExpire/Expires in returned Value. |
| internal/aws/signer/v4/v4.go | Updates signer to call Credentials.Get(ctx) and adjusts documentation. |
| internal/aws/credentials/credentials.go | Redesigns credentials caching around Value.Expired() and atomic storage; removes provider-side IsExpired. |
| internal/aws/credentials/credentials_test.go | Updates credential tests for the new API and concurrency behavior. |
| internal/aws/credentials/chain_provider.go | Updates chain provider to call Retrieve(ctx) and validate returned values. |
| internal/aws/credentials/chain_provider_test.go | Updates chain provider tests for new API/behavior. |
Comments suppressed due to low confidence (1)
x/mongo/driver/auth/aws_sasl_adapter.go:56
- awsSaslAdapter.Next ignores the provided context, and finalMsg creates an http.Request without attaching the SASL context. Because the v4 signer fetches credentials using req.Context(), credential retrieval and signing cannot be canceled when the auth context is canceled.
Thread ctx through step/finalMsg and use http.NewRequestWithContext (or req.WithContext) so signer.Sign and credentials.Get observe cancellation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // AWSCredentialsProvider is the interface used to retrieve AWS credentials. | ||
| type AWSCredentialsProvider interface { | ||
| Retrieve(ctx context.Context) (credentials.Value, error) | ||
| } |
| val := v.(*Value) | ||
| if val == nil || !val.HasKeys() { | ||
| return Value{}, false | ||
| } |
There was a problem hiding this comment.
Agreed, it's unclear why HasKeys should indicate the validity of an AWS credential. This also highlights the danger of using the same credential type for AWS and Azure.
Is Copilot's suggestion an acceptable compromise?
| // Sign will set the request's Body to be the `body` parameter passed in. If an | ||
| // empty body parameter passed to Sign, the request's Body field will be set to | ||
| // nil. Its important to note that this functionality will not change the | ||
| // request's ContentLength of the request. |
| // Validates that a single call to Retrieve is shared between two calls to Get | ||
| time.Sleep(10 * time.Millisecond) | ||
| stub.done <- struct{}{} |
There was a problem hiding this comment.
Agreed, "sleep synchronization" can easily lead to flaky test and should not be used if possible.
matthewdale
left a comment
matthewdale
left a comment
There was a problem hiding this comment.
We need to update the description of this PR to reflect the actual changes and call out that the public options change will be made in the PR that adds the ext/awsauth submodule. As-is, this PR is difficult to review because neither the PR description nor the GODRIVER-3567 definition-of-done suggest why some of the refactors are necessary.
| // Based on github.com/aws/aws-sdk-go by Amazon.com, Inc. with code from: | ||
| // - github.com/aws/aws-sdk-go/blob/v1.44.225/aws/credentials/chain_provider.go | ||
| // See THIRD-PARTY-NOTICES for original license terms | ||
|
|
||
| package credentials |
There was a problem hiding this comment.
We should update the comments in these modified files to indicate that it's no longer a direct copy and has been somewhat significantly modified.
| e.retrieved = true | ||
| if err != nil { | ||
| // Expire the credentials if invalid. | ||
| v.CanExpire = true |
There was a problem hiding this comment.
Optional: Consider also setting v.Expires = time.Time{} to make it explicit that we're marking the expiry time in the past. Currently, that requires knowing that Expires is set to the zero value on line 46. That assumption can also break if the credentials.Value initialization ever changes.
| v.CanExpire = true | |
| v.CanExpire = true | |
| v.Expires = time.Time{} |
|
|
||
| // GetCredentialsDoc generates Azure credentials. | ||
| func (p AzureCredentialProvider) GetCredentialsDoc(ctx context.Context) (bsoncore.Document, error) { | ||
| creds, err := p.cred.GetWithContext(ctx) |
There was a problem hiding this comment.
This seems out-of-scope for this PR, but it's very confusing that an AzureCredentialProvider uses the same AWS credential type and logic used by the AWSCredentialProvider. Do you remember why we use the same cred type for both?
Edit: I see that change was originally made in #1215. Even though I reviewed that PR, it still seems surprising.
| } | ||
|
|
||
| return &MongoDBAWSAuthenticator{ | ||
| credentials: creds.NewAWSCredentialProvider(httpClient, providers...).Cred, |
There was a problem hiding this comment.
Optional: Now that we've added the new AWSCredentialsProvider interface, the naming of AWSCredentialProvider creates a lot of confusion. Is it possible to rename that type, or move it to the mongocrypt package since it's only actually used to implement the kmsProvider interface?
qingyang-hu
changed the title
GODRIVER-4034
Summary
This ticket re-shapes the built-in AWS credential interfaces to match aws-sdk-go-v2:
ValuedropsProviderNameand gains the SDK-v2 fields:Source,CanExpire,ExpiresandAccountID. Exactly the field set specified for the Retrieve return struct in:The old Provider interface had
Retrieve() (Value, error)plusIsExpired() bool, with a separateProviderWithContextshim forRetrieveWithContext(ctx).These migrate into one method:
Retrieve(ctx context.Context) (Value, error). Every concrete provider (assume_role, ec2, ecs, env, imds, static) is updated to the new signature.A new
Value.Expired()method derives expiry from Expires/CanExpire. Providers/chains no longer track expiry separately.Background & Motivation
This is a prerequisite ticket with no-public-API changes only. It de-risks the feature by landing the mechanical interface alignment on its own. The follow-up PRs will add new surface: the options interface of credential/encryption setters and the ext/awsauth adapter.